Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

More thorough testing of BTreeMap::range #67758

Merged
merged 1 commit into from
Jan 19, 2020
Merged

More thorough testing of BTreeMap::range #67758

merged 1 commit into from
Jan 19, 2020

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Dec 31, 2019

Test more of the paths in the range_search function in map.rs

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 31, 2019
@ssomers
Copy link
Contributor Author

ssomers commented Dec 31, 2019 •
edited
Loading

src/liballoc/collections/btree/map.rs line 1955 is currently the start of loop of the range_search function. There were no tests hitting the match variants at line 1968, 1972 and 1992. With this testing, 1968 is hit but the other two "(true, Excluded(_))" never are. I don't understand the code well so I was unable to hit them, or to figure out that they are perhaps impossible. all are hit.

@ssomers
Copy link
Contributor Author

ssomers commented Dec 31, 2019

It was bound to happen... I had another look and added a test case to cover everything.

Note I also let test_range_large take on a more modest tree, and I don't know if it's useful at all. There's also test_range that tries all positions for the Inclusive search, and test_range_1000 for those who believe that testing on bigger things is better testing.

RalfJung
RalfJung reviewed Jan 9, 2020
View reviewed changes
src/liballoc/tests/btree/map.rs

#[test]
fn test_range_large() {
let size = 200;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try running this in Miri? I think it'll be too slow, and it's probably best to disable this test in Miri (or maybe use a smaller size in Miri, one that doesn't take forever).

Copy link
Contributor Author

@ssomers ssomers Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I always ask Miri. In fact I was relying on Miri a bit too much lately.

It's not a new test, but renamed from _internal _inclusive, and it was bigger before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, seems like Miri is fast enough then, great. :)

In fact I was relying on Miri a bit too much lately.

How that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not Miri itself, actually. Just the fact that I have Miri set up to run only the src/liballoc tests and forget all the rest on Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, seems like Miri is fast enough then, great. :)

It was dragging through the test at 500, but I really don't see the point in testing something with 500 elements when 144 would give you the same 3-level search. More? Why - to have a filled up node somewhere? Then 150 would be enough. For starters, I wish we could check a tree's height and/or read that node.CAPACITY or node.B value.

@ssomers ssomers changed the title more thorough testing of BTreeMap::range More thorough testing of BTreeMap::range Jan 9, 2020
@Mark-Simulacrum
Copy link
Member

@bors r+
r? @Mark-Simulacrum

Seems fine to add more tests to BTreeMap, since this essentially just checks current behavior rather than changing it.

@bors
Copy link
Contributor

bors commented Jan 19, 2020

📌 Commit 8314b7f has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2020
@bors
Copy link
Contributor

bors commented Jan 19, 2020

⌛ Testing commit 8314b7f with merge 6250d56...

bors added a commit that referenced this pull request Jan 19, 2020
@bors
Auto merge of #67758 - ssomers:testing_range, r=Mark-Simulacrum
6250d56 
More thorough testing of BTreeMap::range

Test more of the paths in the `range_search` function in map.rs
@bors
Copy link
Contributor

bors commented Jan 19, 2020

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 6250d56 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 19, 2020
@bors bors merged commit 8314b7f into rust-lang:master Jan 19, 2020
@bors bors mentioned this pull request Jan 19, 2020
Merged
@ssomers ssomers deleted the testing_range branch January 19, 2020 21:39
@ssomers ssomers mentioned this pull request Jan 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ssomers @rust-highfive @Mark-Simulacrum @bors @RalfJung @joshtriplett